Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BSIP40 Implementation #1860

Merged
merged 42 commits into from
Oct 21, 2019
Merged

Conversation

nathanielhourt
Copy link
Contributor

My implementation for BSIP 40, Custom Active Authorities

A couple of notes:

  • Equality comparison between two containers of mismatched types is not working because I can't get the SFINAE to compile... so comparing a flat_set<T> against a vector<T>, for instance, doesn't work.
  • The fee logic is very simple, just a flat op price plus a price per byte. I believe this structure is adequate, and its simplicity will significantly reduce the complexity of frontend development.

@abitmore
Copy link
Member

Equality comparison between two containers of mismatched types is not working because I can't get the SFINAE to compile... so comparing a flat_set against a vector, for instance, doesn't work.

Fortunately we only have flat_set used in operations? Otherwise I guess we need to add more types into GRAPHENE_OP_RESTRICTION_ARGUMENTS_VARIADIC.

The fee logic is very simple, just a flat op price plus a price per byte. I believe this structure is adequate, and its simplicity will significantly reduce the complexity of frontend development.

The front-end usually doesn't calculate fees by their own but simply calls get_required_fees API, the real work will still be done by the back end.

libraries/chain/db_notify.cpp Show resolved Hide resolved
libraries/chain/db_notify.cpp Outdated Show resolved Hide resolved
libraries/chain/db_notify.cpp Outdated Show resolved Hide resolved
libraries/protocol/custom_authority.cpp Show resolved Hide resolved
libraries/protocol/custom_authority.cpp Outdated Show resolved Hide resolved
libraries/chain/custom_authority_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/custom_authority_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/custom_authority_evaluator.cpp Outdated Show resolved Hide resolved
@nathanielhourt
Copy link
Contributor Author

Equality comparison between two containers of mismatched types is not working because I can't get the SFINAE to compile... so comparing a flat_set against a vector, for instance, doesn't work.

Fortunately we only have flat_set used in operations? Otherwise I guess we need to add more types into GRAPHENE_OP_RESTRICTION_ARGUMENTS_VARIADIC.

Vectors are used in several operations. Mostly vector<char> though there are a few others. Comparing a vector to a flat_set is clumsy, so it doesn't need to work, but perhaps we should add some vectors to the legal restriction arguments...

@abitmore abitmore mentioned this pull request Jul 20, 2019
14 tasks
libraries/chain/custom_authority_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/custom_authority_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/custom_authority_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/custom_authority_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/custom_authority_evaluator.cpp Outdated Show resolved Hide resolved
@nathanielhourt
Copy link
Contributor Author

@MichelSantos has brought up an interesting point: the current spec/implementation does not really provide a way to restrict that a container field have only values selected from a defined list. The logical way to do this would be to allow the in predicate (any in the spec) to operate on a container type field as well as a scalar type field, to say "all values in the field must be in the argument". This could be done for the not_in predicate (none in the spec) as well, to say "no value in the field may be in the argument".

With a slightly loose interpretation of the wording, the spec supports this. Shall I add this functionality to the implementation?

@nathanielhourt
Copy link
Contributor Author

Changes requested so far have been applied.

tests/tests/custom_authority_tests.cpp Outdated Show resolved Hide resolved
libraries/chain/proposal_evaluator.cpp Outdated Show resolved Hide resolved
@pmconrad
Copy link
Contributor

Shall I add this functionality to the implementation?

I tend to leaving this for a later release. Unsure though. I guess it's a small change only, but OTOH I can't imagine where it'd be useful.

@nathanielhourt
Copy link
Contributor Author

Shall I add this functionality to the implementation?

I tend to leaving this for a later release. Unsure though. I guess it's a small change only, but OTOH I can't imagine where it'd be useful.

It is fairly straightforward to add, yeah. Michel was attempting to test a case where someone wants to set a custom authority that can appoint feed producers for an asset from an authorized list. I'm not sure how useful that is in real life, but it's an illustrative example.

Add specializations to allow `in` and `not_in` restrictions to operate
on all values in a container field
@nathanielhourt
Copy link
Contributor Author

I've added the specializations, but I can remove them if they are not desired.

Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, thanks!

@sschiessl-bcp
Copy link

Awesome!

Copy link
Contributor

@jmjatlanta jmjatlanta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiled on Ubutntu 18.04 / Boost 1.65.1 / OpenSSL 1.1.1. chain_tests ran without errors. Code looks good.

@nathanielhourt nathanielhourt merged commit f182b60 into bitshares:hardfork Oct 21, 2019
@nathanielhourt nathanielhourt deleted the bsip40 branch October 21, 2019 18:21
@clockworkgr
Copy link
Member

Yay!...this once lowly issue: #1061 led to this :)

Somewhat proud despite not actually having done much :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

7 participants